llvm-openmp: fix incompatibilities with official FindOpenMP, add v18.1.8, simplify patches, add MSVC support
#24584
Conversation
The wrong runtime library gets picked up in test_package for some reason. Don't feel like debugging it.
…uring packaging Avoids a fragile compilation check during find_package().
|
I think I'm going to revert the |
|
To address the comments by @jcar87 at #22353 (comment): I think most of the comments miss the mark a bit and approach it from the vantage point of trying to use I'll be skipping the points that I already covered in the issue description. Correct me if I miss anything. In general, all OpenMP runtimes implement the same API and some of the stable extensions of each other (e.g. from GOMP), so they are intended to be mutually compatible. While that does not seem reliable enough to be used as the general default, which I agree with, I don't see a reason to disable this choice for users who are aware of what they are doing from using the libomp runtime with other compilers.
Unless a Conan user goes out of their way to use Conan in a more exotic fashion, Conan packages are isolated and the include dirs are not placed on a globally visible path that might trip up GCC, unless the user is explicitly linking against the targets in this package.
While I can't say if this is dependent on the order of the build flags, as the test_package demonstrates and after using this recipe for tens of packages with GCC, the only time The user will have to take care not to accidentally link against external or system libraries that link against libgomp, no doubt. I can add a warning when using the package with non-Clang/AppleClang/MSVC compilers, perhaps. The project also includes an option to create symlinks aliases for
Again, if the user is specifically using
I intentionally disregarded that option for the reason mentioned above. I could add an explicit comment in the wrapper about that, though.
I'll take care of that, sure. The only copied parts are the detection of the detection of the OpenMP version supported by the preprocessor and mapping that to spec date. The rest is original.
I mean... you answered that yourself - it's definitely needed for static libraries. You will either need to pass
I agree and understand. I'm definitely open to a better solution.
That's a very good point. I'll change the default to |
This comment has been minimized.
This comment has been minimized.
|
While I'm still not advocating for using LLVM OpenMP in place of GOMP or other native OpenMP runtimes, an interesting data point in that regard is conda-forge. While the main Anaconda channel uses gomp as its OpenMP implementation (https://anaconda.org/anaconda/_openmp_mutex/files), conda-forge adds support for The LLVM variant of The symlinking of |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This adds an implicit -lomp, which generally fails due to llvm-openmp not being added to linker paths. It's better to fail immediately. An example: https://github.com/opencv/opencv/blob/4.10.0/cmake/OpenCVFindFrameworks.cmake#L33-L34
This comment has been minimized.
This comment has been minimized.
| # Setting the flags for internal use only. | ||
| set(_OpenMP_FLAGS "@OpenMP_FLAGS@") | ||
|
|
||
| set(OpenMP_C_FLAGS "--dont-use-OpenMP_C_FLAGS--use-OpenMP::OpenMP_C-target-instead") |
There was a problem hiding this comment.
This is ugly, but a potential alternative of passing the appropriate -I/-L paths together with -fopenmp did not work due to the -L flag not getting propagated to the linker when passed via CMAKE_C_FLAGS, which is how this is typically used.
Fortunately, most modern CMake projects use the OpenMP targets instead and the rest can be patched quite easily. Here's a list of such projects (ticked ones have a patch available): https://gist.github.com/valgur/cbd1a856a2eb42c285a2360ecceb0405
The only project that truly needed the flags variable instead of CMake targets was VTKm: https://gitlab.kitware.com/vtk/vtk-m/-/blob/master/CMake/VTKmDeviceAdapters.cmake#L64
This comment has been minimized.
This comment has been minimized.
valgur
force-pushed
the
bugfix/llvm-openmp
branch
from
8923869 to
6d2542a
Compare
Conan v1 pipeline ✔️All green in build 13 (
Conan v2 pipeline ✔️
All green in build 13 (
|
Summary
Changes to recipe: llvm-openmp/*
Motivation
See #24577 for more context. Re-opens #22353.
The
llvm-openmprecipe exports aFindOpenMP.cmakemodule with the appropriateOpenMP::OpenMP_CXXandOpenMP::OpenMP_Ctargets, but is otherwise not compatible with the output variables and version set by the officialFindOpenMP.cmakemodule.Here's the current output for
find_package(OpenMP)withllvm-openmp:Here's the output with the fixes addressing this issue from this PR applied:
Many (typically older) projects rely on these variables being available. The official module also supports
C,CXXandFortrancomponents in thefind_package()call, which the exported module ignores.Details
Possible solutions to the incompatibilities are:
validate()and create a thin wrapper around CMake'sFindOpenMP.cmake. This only works for Clang/AppleClang because CMake'sFindOpenMP.cmakealways selects the compiler's native OpenMP runtime to link against and would lead to mixing oflibompand the native runtime in transitive dependencies, which is not safe. Note that CMake'sFindOpenMP.cmakeassumes that the runtime library is on the compiler's default search paths, so creating such a wrapper might not be as simple as it looks at first glance and the only real benefit ends up being not including the OpenMP version detection logic in the wrapper directly. Getting the externaltry_compile()to work reliably is tricky, though, and the picked OpenMP flags might accidentally differ from the ones propagated bypackage_info(), so I'm not a fan of this approach.FindOpenMPsomewhat, but not by a significant amount since it does not need to be nearly as generic. This will work as expected with all compilers - use the compiler's preprocessor for the translation andlibompas the runtime. TheFindOpenMPinterface has been very stable as well and can easily be updated if it does changes (as it did in 3.30). This is the approach that was proposed in llvm-openmp: match FindOpenMP.cmake output, add MSVC support #22353.I'm fine with either approach, ultimately, but lean towards the latter. Not because I think that other compilers should use
libompfor whatever the reason, but rather because I don't see a reason to limit the use of the package to providelibompunnecessarily if the consumer has a specific reason to prefer it and knows what they are doing.I have tested this recipe with tens of different packages on my private fork of CCI: https://github.com/valgur/conan-center-index
The only real failure case is when an external project does just
compile_options(${OpenMP_CFLAGS})or similar, since passing only-fopenmpto the compiler without adding the includes and runtime linking is not sufficient. Unfortunately, there is no good fix for this unless I add an ugly switch for all possible compilers and add the equivalent of-I... -L... -lompto the flags. Patching such projects to uselink_libraries(OpenMP::OpenMP_C)and submitting the patch upstream as well is probably a much better way to handle this.TODO:
libomp.dll->omp.dll.shared=True.OpenMP_RUNTIME_MSVCis intentionally not applied. Maybe raise an error when it's set to a value other thanllvm.